Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(table): Use extras in queries #30335

Conversation

Antonio-RiveroMartnez
Copy link
Member

SUMMARY

Table chart was removing the extras from the summary query, assuming they were only used for time grains. This PR fixes that and put the extras back in the queryObject sent as payload, now, only excluding time_grain_sqla if no temporal column is included in your chart.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before: Excluding extras from summary query and only adding it if no time comparison was used
before

Now: Sending extras in the total query regardless you're using time comparison
No temporal Column
now_no_temp
With temporal Column
now_temp

TESTING INSTRUCTIONS

  1. Create a Table Chart
  2. Use no temporal column and a custom SQL filter
  3. Mark the Show summary checkbox
  4. The queries sent as payload must include the where in extras for both queries and not include a time_grain_sqla
  5. Now add a temporal column to your chart and run the query again
  6. The time_grain_sqla must be present in the extras alongside with the where clause

ADDITIONAL INFORMATION

  • Has associated issue: show summary does not correctly apply all filters #30193
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@Antonio-RiveroMartnez Antonio-RiveroMartnez requested a review from a team September 19, 2024 11:48
@dosubot dosubot bot added change:frontend Requires changing the frontend viz:charts:table Related to the Table chart labels Sep 19, 2024
@Antonio-RiveroMartnez Antonio-RiveroMartnez changed the title fix(table): Use extras in query only remove time_grain_sqla when not needed fix(table): Use extras in queries Sep 19, 2024
Copy link
Contributor

@giftig giftig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestions but LGTM

@Antonio-RiveroMartnez Antonio-RiveroMartnez merged commit 6c2bd2a into apache:master Sep 19, 2024
33 checks passed
@sadpandajoe sadpandajoe linked an issue Sep 19, 2024 that may be closed by this pull request
3 tasks
@sadpandajoe sadpandajoe added the v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch label Sep 19, 2024
sadpandajoe pushed a commit that referenced this pull request Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:frontend Requires changing the frontend plugins size/M v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch viz:charts:table Related to the Table chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

show summary does not correctly apply all filters
3 participants